Skip to content

test: formalize classic S7 real PLC acceptance testing - #892

Open
gijzelaerr wants to merge 2 commits into
masterfrom
feat/840-real-plc-acceptance
Open

gijzelaerr wants to merge 2 commits into
masterfrom
feat/840-real-plc-acceptance

Conversation

@gijzelaerr

@gijzelaerr gijzelaerr commented Sep 11, 2026

Copy link
Copy Markdown
Owner

Progresses #840.

Adds a pytest-bdd-based real-PLC acceptance program with 13 versioned scenarios covering connection, canonical reads, multi-read, reconnect behavior, and safely restored scratch writes using the classic S7 protocol.

Safety and evidence:

  • smoke-only default runner; writes/admin require separate explicit opt-ins
  • shared classic-S7 helpers used by the existing diagnostic e2e suite
  • scratch bytes restored and read-back verified from fixture teardown
  • Gherkin terminal output, JUnit XML, and schema-versioned sanitized JSON
  • issue form, saved searches, fixture setup, and volunteer runbook
  • real-PLC tests remain opt-in and outside hosted CI

Local verification:

  • uv run --no-sync pre-commit run --all-files
  • uv run --no-sync pytest (1385 passed, 63 skipped)
  • uv build --offline
  • uv run --no-sync sphinx-build -N -W -b html doc ...

The final acceptance item—filing a real-hardware result—requires a volunteer PLC run after this infrastructure lands, so this PR intentionally does not close #840.

@gijzelaerr

Copy link
Copy Markdown
Owner Author

@russwing Could you run the cross-model real-PLC acceptance pass for the S7CommPlus work once its code review settles? Your S7-1200 and S7-1500 access makes that the useful compatibility pair. Suggested order: the response/authentication roots (#881, #882, #883, #887), then stacked integrity/renewal (#884, #888), followed by typed tags/subscriptions (#886, #891). Please use the result template in this PR and record the exact CPU, firmware, protection mode, and tested commit. #881 still has an unresolved S7-1511C item-read failure from the latest reporter retest, so a failure there would also be valuable evidence.

@russwing

Copy link
Copy Markdown
Contributor

Ran the harness today on both CPUs of the compatibility pair. One blocking defect in the S7CommPlus adapter, and a preparation gap of ours. Commit eb6d9e3.

The blocker: the S7CommPlus adapter never enables TLS

tests/real_plc/support.py::S7CommPlusAdapter.connect calls:

self.client.connect(self.config.host, self.config.port, self.config.rack, self.config.slot)

S7CommPlusClient.connect defaults use_tls: bool = False, and its own docstring says "required for V2". S7CommPlus V2 is mandatory-TLS — S7-1200 firmware V4.5+ and S7-1500 V2.x+ — so the adapter cannot reach any modern CPU. Every s7commplus scenario fails at connect with S7ConnectionError: Connection closed by peer, which is the RST an unencrypted V1 SetupSession earns from a V2 target.

Controlled proof — same CPU, same commit, same port, only use_tls differs:

S7-1200 1212C V4.7.3     use_tls=False -> S7ConnectionError: Connection closed by peer
S7-1200 1212C V4.7.3     use_tls=True  -> CONNECTED protocol_version=V2
S7-1500 1511F V2.9.7     use_tls=False -> S7ConnectionError: Connection closed by peer
S7-1500 1511F V2.9.7     use_tls=True  -> CONNECTED protocol_version=V2

Suggested fix: a --plc-use-tls flag (plus --plc-tls-cert/key/ca passthrough) on run_real_plc_acceptance.py, threaded into the adapter, and recorded in the report metadata since it is part of the tested security mode. Without it the acceptance program can only certify pre-V4.5 S7-1200s and V1 S7-1500s.

Results as they stand

protocol S7-1200 1212C V4.7.3 S7-1500 1511F-1 PN V2.9.7
legacy_s7 Establish and close a session — PASS Establish and close a session — PASS
s7commplus all fail at connect (no TLS) all fail at connect (no TLS)

The other four scenarios fail on both CPUs for a reason that is ours, not yours: neither PLC carries the DB1/DB2 fixture from tests/plc_setup/e2e_test_dbs.scl. Importing it needs TIA Portal and our automation engineer; it is requested and should land with his next session, after which I can run the full suite including --allow-write.

So this is a partial, and I would rather report it now than sit on the TLS finding.

One note on hardware scope

#881's outstanding item-read failure is on an S7-1511C. Ours is a 1511F — same S7-1500 family, different model and a safety variant. Whatever we produce is useful compatibility evidence but will neither confirm nor clear that specific report.

I will file the formal Real PLC test result issues with both artifacts once the fixture DBs are in and the TLS path is reachable — filing a result now would mostly record a harness gap rather than PLC behaviour.

@russwing

Copy link
Copy Markdown
Contributor

Fixture DBs are now in place on both CPUs, so here is the full pass. Two defects, one of them substantial, plus confirmation that the TLS fix works.

Harness eb6d9e3, plus a one-line local patch described below. Both CPUs are dedicated bench units.

Results

S7-1200 1212C V4.7.3 S7-1500 1511F-1 PN V2.9.7
legacy_s7 smoke 5/5 PASS 1/5 — PUT/GET is disabled (class=0x81 code=0x04, service not implemented). A CPU setting, not a library issue.
s7commplus smoke, stock 0/5 — cannot connect 0/5 — cannot connect
s7commplus smoke, TLS patched 1/5 — session PASSES, all reads fail 1/5 — session PASSES, all reads fail

1. The TLS fix works — confirmed

Patching S7CommPlusAdapter.connect to pass use_tls=True turns the connection scenario from fail to PASS on both CPUs. That confirms the fix proposed in my earlier comment. Please add --plc-use-tls (plus cert/key/ca passthrough) and record it in the report metadata.

2. db_read over S7CommPlus ignores offset and size

This is the bigger one, and it is not fixed by #881 — I merged #881 into the harness branch and re-ran; byte-for-byte identical failure on both CPUs.

Same CPU (1212C), same DB, same offsets, both protocol paths side by side:

DB1.0  size= 2   legacy=00 0a                     s7cp=92 00 00 01
DB1.0  size= 8   legacy=00 0a 00 ff 42 f6 e6 66   s7cp=92 00 00 01
DB1.2  size= 4   legacy=00 ff 42 f6               s7cp=42
DB1.0  size=16   legacy=<16 correct bytes>        s7cp=92 00 00 01

Three separate problems visible in four lines:

  • Size is ignored. Requests for 2, 8 and 16 bytes all return the same four bytes.
  • The data does not match the block. 92 00 00 01 appears nowhere in DB1's real contents.
  • Offset is ignored or mis-scaled. DB1.2 size=4 returns a single byte 42, which is the byte at DB1.4.

The legacy column is ground truth from the same CPU seconds apart, so the fixture and the DB are demonstrably correct.

Identical bytes on two different CPU families (1212C V4.7.3 and 1511F V2.9.7) points at client-side decode rather than anything CPU-specific.

On #881

You flagged an unresolved S7-1511C item-read failure and said a failure would be useful evidence. We reproduce a read failure on a 1511F and a 1212C, and #881 as it stands does not resolve either. Whether it is the same root cause as the 1511C report I cannot say — different model, and I do not have that reporter's fixture.

Given the size/offset behaviour above, I would treat this as distinct from "unprefixed response values": the transport is not returning a wrong-shaped version of the right data, it is returning the wrong data.

Scope notes

  • No --allow-write run yet. DB2 exists and writes are cleared on these bench units, but running scratch writes over a path whose reads are this wrong would tell us little and risks writing somewhere unintended. Happy to run writes once reads are trustworthy.
  • I have not filed formal Real PLC test result issues yet — the current results mostly characterise two library defects rather than PLC behaviour, and I would rather file a clean result once the TLS flag lands and reads are fixed. Say if you would prefer them filed as-is and I will.
  • PUT/GET on the 1500 can be enabled if you want the legacy path covered on that family; it is off by default there and the runbook rightly says not to weaken a PLC without cause.

@gijzelaerr gijzelaerr added this to the 3.2 milestone Sep 20, 2026
@gijzelaerr gijzelaerr modified the milestones: 3.2, 3.3 Sep 20, 2026
@gijzelaerr gijzelaerr changed the title test: formalize real PLC acceptance testing test: formalize classic S7 real PLC acceptance testing Sep 20, 2026
@gijzelaerr

gijzelaerr commented Sep 20, 2026

Copy link
Copy Markdown
Owner Author

The S7CommPlus portion has now moved to the standalone repository in gijzelaerr/s7commplus#30; this PR remains the Classic S7 acceptance work.

That port addresses both findings from the hardware pass:

  • TLS is configurable with --plc-use-tls and certificate/key/CA passthrough, including the existing E2E path. Reports capture negotiated TLS plus boolean client-certificate/CA usage without exposing paths or the PLC address.
  • Raw DB access now sends the ClassicBlob marker, a zero-based byte offset, and an independent byte size. BLOB PValues include BlobRootId, and raw writes use the BLOB wire type. The exact paths and values are covered by byte-level and emulator regressions for sync/async clients.
  • The canonical DB1/DB2 SCL fixture is included in that repository, so fixture preparation is no longer external to the test source.

The standalone PR merged with all hosted checks green (805 passed, 43 hardware-gated skips locally, plus typing, lint/format, docs, package/Twine, lock, and pre-commit checks). I have deliberately not treated unit/emulator coverage as proof on hardware. The next useful step is to rerun S7CommPlus master on the same 1212C V4.7.3 and 1511F V2.9.7, first smoke and then --allow-write only after reads return the canonical bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formalize real-PLC acceptance testing with Gherkin scenarios

2 participants